github api retries #108
github api retries #108
Conversation
WalkthroughThe changes introduce a new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (10)
test/github/mock_api.py (3)
1-2: Document dependencies properly using requirements.txt or pyproject.tomlInstead of using an inline comment, document the FastAPI dependency properly in a requirements file.
Would you like me to help create a proper dependency specification file?
7-9: Add type hints and docstringConsider adding type hints to global variables and a module docstring explaining the purpose of this mock server.
+"""Mock GitHub API server for testing retry mechanisms.""" + app = FastAPI() -repo = {"default_branch": "default_branch"} -_globals = {"retries":1} +repo: dict[str, str] = {"default_branch": "default_branch"} +_globals: dict[str, int] = {"retries": 1}
1-28: Add security measures for the mock serverSince this is a mock server for testing, ensure it:
- Only binds to localhost
- Uses a random available port
- Has appropriate warnings in the documentation about not using in production
Would you like me to provide an example of how to implement these security measures?
🧰 Tools
🪛 Ruff
4-4:
pydantic.BaseModelimported but unusedRemove unused import:
pydantic.BaseModel(F401)
test/github/test_github_api.sh (2)
16-19: Add error context to build step.While the build step is correct, it would be more maintainable to add context to potential failures.
( cd ../.. - mkn clean build -p github -dtOa -fPIC + mkn clean build -p github -dtOa -fPIC || { echo "Failed to build github profile"; exit 1; } )
36-39: Add error context to test execution.Similar to the build step, add context to potential failures.
( cd ../.. - mkn run -p github + mkn run -p github || { echo "GitHub API tests failed"; exit 1; } )test/github/test.cpp (2)
1-2: Remove unnecessary empty lines at the start of the file.- - #include "maiken/github.hpp"
9-13: Improve static initialization block readability and documentation.The static initialization uses a lambda with a non-descriptive variable name. Consider:
- Adding a comment explaining why we're using localhost:8000
- Using a more descriptive variable name
-bool static const premain = []() { +// Initialize GitHub API endpoint to point to local mock server for testing +bool static const init_github_test_config = []() { github::URL = "localhost"; github::port = 8000; return true;inc/maiken/github.hpp (3)
37-37: Organize included headers for clarityConsider grouping the standard library headers and third-party library headers separately for better readability.
You can group the headers like this:
#include "maiken.hpp" +#include <chrono> +#include <string> +#include <thread> #ifdef _MKN_WITH_MKN_RAM_ #include "mkn/kul/yaml.hpp" #include "mkn/ram/http.hpp" #include "mkn/ram/https.hpp"
81-105: Avoid magic numbers and use constants insteadThe retry count
3and the sleep duration50msare hard-coded. Using named constants improves code maintainability.Define constants for retry count and sleep duration:
constexpr int MAX_RETRIES = 3; constexpr auto RETRY_DELAY = std::chrono::milliseconds(50);Then use them in your code:
int retry = MAX_RETRIES; // ... std::this_thread::sleep_for(RETRY_DELAY);
208-210: Check the return value of API callsIn the loop, if an API call fails, it proceeds to the next without logging the failure reason.
Add logging to inform which method failed:
for (auto const& order : orders) { if (gets[order](owner, repo, branch)) { return true; } else { KLOG(WARN) << "Method " << order << " failed to retrieve branch information."; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
test/github/__pycache__/mock_api.cpython-312.pycis excluded by!**/*.pyc
📒 Files selected for processing (7)
- inc/maiken/github.hpp (2 hunks)
- mkn.yaml (2 hunks)
- src/maiken/scm/github.cpp (0 hunks)
- test/github/mock_api.py (1 hunks)
- test/github/requirements.txt (1 hunks)
- test/github/test.cpp (1 hunks)
- test/github/test_github_api.sh (1 hunks)
💤 Files with no reviewable changes (1)
- src/maiken/scm/github.cpp
🧰 Additional context used
🪛 Ruff
test/github/mock_api.py
4-4:
pydantic.BaseModelimported but unusedRemove unused import:
pydantic.BaseModel(F401)
🪛 Shellcheck
test/github/test_github_api.sh
[warning] 8-8: exit_code appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 12-12: rv is referenced but not assigned.
(SC2154)
🔇 Additional comments (3)
test/github/test_github_api.sh (1)
1-5: LGTM! Script setup follows best practices.The initialization is well-structured with proper shebang, working directory setup, and error tracing.
mkn.yaml (1)
6-6: LGTM: Version date updated.The DATE property has been updated to reflect the current version.
inc/maiken/github.hpp (1)
181-218: Ensure compatibility when SCM is disabledWhen
_MKN_DISABLE_SCM_is defined, the function returns0after logging an error. Ensure that this behavior is intentional and does not cause issues elsewhere.Confirm that calling
KEXIT(1, ...)aligns with the expected behavior when SCM is disabled.✅ Verification successful
Let me gather more information about the KEXIT macro definition and usage.
Let me search for the KEXIT macro definition to understand its behavior.
Let me try a different approach to find the error handling mechanism.
Based on the evidence gathered, I can now provide a final response about the KEXIT behavior when SCM is disabled.
The error handling is consistent with the codebase's behavior
The use of
KEXIT(1, ...)when SCM is disabled is intentional and consistent with the error handling pattern throughout the codebase. When SCM functionality is disabled:
- The error message clearly indicates why the operation failed ("SCM disabled, cannot resolve dependency...")
- The exit code 1 is used consistently across the codebase to indicate error conditions
- This prevents silent failures and ensures users are explicitly informed about the disabled functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the code handles _MKN_DISABLE_SCM_ correctly. # Test: Search for occurrences of KEXIT in the codebase to ensure proper usage. rg 'KEXIT\('Length of output: 9972
Script:
#!/bin/bash # Search for KEXIT definition and related macros rg -A 5 'KEXIT|#define.*KEXIT' # Search for files that might contain error handling utilities fd -e hpp -e h "err|error|exit" --exec cat {}Length of output: 37814
Script:
#!/bin/bash # Search for KEXIT macro definition rg -p '#define.*KEXIT' # Look for error handling header files fd -e hpp -e h "error|exit" -x cat {} | rg -A 5 'KEXIT|#define.*KEXIT'Length of output: 99
Script:
#!/bin/bash # Search for header files that might contain the KEXIT definition fd -e hpp -e h -x cat {} | rg -B 5 -A 2 'KEXIT|kul.*exit|Exception' # Search for any error handling related includes rg '#include.*err|exit'Length of output: 20118
| pydantic | ||
| uvicorn | ||
| fastapi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider separating test dependencies.
Since these dependencies are specifically for testing purposes (mock API server), consider moving them to a dedicated test requirements file (e.g., test-requirements.txt or requirements-test.txt). This helps maintain a clear separation between production and test dependencies.
Pin dependency versions for stability.
To prevent breaking changes and ensure reproducible builds, consider pinning specific versions:
-pydantic
-uvicorn
-fastapi
+pydantic==2.5.2
+uvicorn==0.24.0.post1
+fastapi==0.104.1Committable suggestion was skipped due to low confidence.
| @app.get("/repos/owner/repo_invalid_response") | ||
| async def repo_invalid_response(): | ||
| return "NOT JSON" |
There was a problem hiding this comment.
Fix invalid JSON response simulation
FastAPI automatically JSON-encodes responses. To properly simulate an invalid JSON response, use the Response class.
+from fastapi import Response
+
@app.get("/repos/owner/repo_invalid_response")
async def repo_invalid_response():
- return "NOT JSON"
+ return Response(
+ content="NOT JSON",
+ media_type="text/plain",
+ status_code=status.HTTP_500_INTERNAL_SERVER_ERROR
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @app.get("/repos/owner/repo_invalid_response") | |
| async def repo_invalid_response(): | |
| return "NOT JSON" | |
| from fastapi import Response | |
| @app.get("/repos/owner/repo_invalid_response") | |
| async def repo_invalid_response(): | |
| return Response( | |
| content="NOT JSON", | |
| media_type="text/plain", | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR | |
| ) |
| @app.get("/repos/owner/repo_fail") | ||
| async def repo_fails(): | ||
| return {} |
There was a problem hiding this comment.
Improve error response simulation
The current implementation doesn't accurately simulate GitHub API error responses. GitHub API returns specific error structures with appropriate status codes.
-@app.get("/repos/owner/repo_fail")
+@app.get("/repos/owner/repo_fail",
+ status_code=status.HTTP_404_NOT_FOUND,
+ description="Simulates a failed GitHub API response")
async def repo_fails():
+ """Simulate a GitHub API error response."""
- return {}
+ return {
+ "message": "Not Found",
+ "documentation_url": "https://docs.github.com/rest/reference/repos#get-a-repository"
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @app.get("/repos/owner/repo_fail") | |
| async def repo_fails(): | |
| return {} | |
| @app.get("/repos/owner/repo_fail", | |
| status_code=status.HTTP_404_NOT_FOUND, | |
| description="Simulates a failed GitHub API response") | |
| async def repo_fails(): | |
| """Simulate a GitHub API error response.""" | |
| return { | |
| "message": "Not Found", | |
| "documentation_url": "https://docs.github.com/rest/reference/repos#get-a-repository" | |
| } |
|
|
||
|
|
||
| #include "maiken/github.hpp" | ||
| #include "mkn/kul/assert.hpp" | ||
|
|
||
| #include <cstdint> | ||
|
|
||
| namespace maiken { | ||
| bool static const premain = []() { | ||
| github::URL = "localhost"; | ||
| github::port = 8000; | ||
| return true; | ||
| }(); | ||
|
|
||
| } // namespace maiken | ||
|
|
||
| int test_repo() { | ||
| std::string branch; | ||
| bool b = maiken::Github<false>::GET_DEFAULT_BRANCH("owner", "repo", branch); | ||
| mkn::kul::abort_if(!b); | ||
| return !b; | ||
| } | ||
|
|
||
| int test_repo_fail() { | ||
| std::string branch; | ||
| bool b = maiken::Github<false>::GET_DEFAULT_BRANCH("owner", "repo_fail", branch); | ||
| mkn::kul::abort_if(b); | ||
| return b; | ||
| } | ||
|
|
||
| int test_repo_invalid_response() { | ||
| std::string branch; | ||
| bool b = maiken::Github<false>::GET_DEFAULT_BRANCH("owner", "repo_invalid_response", branch); | ||
| mkn::kul::abort_if(b); | ||
| return b; | ||
| } | ||
|
|
||
| int main(int argc, char* argv[]) { // | ||
| return test_repo() + test_repo_fail() + test_repo_invalid_response(); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Test setup is well-structured but needs documentation improvements
The test setup is properly implemented with:
- A FastAPI-based mock server (
mock_api.py) that simulates GitHub API responses - A test script (
test_github_api.sh) that handles server startup/teardown - Test cases covering success, failure, and invalid response scenarios
However, a few improvements are needed:
- Add documentation in
test/github/README.mdexplaining the mock server setup and test requirements - Document the FastAPI dependency in
requirements.txt - Consider adding comments in
test.cppto explain the purpose of the premain block
🔗 Analysis chain
Verify test setup documentation and consistency.
Let's ensure the test setup is properly documented and consistent with other tests in the codebase.
Let me gather more information about the test setup and documentation.
Let me check the mock API setup and test script contents to complete the verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's documentation about the mock API setup
fd README.md test/github/
# Look for similar test patterns in other test files
rg -l "Github.*GET_DEFAULT_BRANCH" "test/"
# Check for other test files using the same port
rg "port\s*=\s*8000" "test/"
Length of output: 164
Script:
#!/bin/bash
# Check for any documentation about testing in the repository
fd README.md
# Look for mock server or test setup documentation
rg -i "mock.*server|test.*setup" -g "!{*.cpp,*.o,*.a}"
# Check if there are other test files in github directory
fd . test/github/
# Check how the Github URL is used in other files
rg "github::URL" --type cpp
Length of output: 537
Script:
#!/bin/bash
# Check mock API implementation
cat test/github/mock_api.py
# Check test script implementation
cat test/github/test_github_api.sh
# Check if there are any testing instructions in README
rg -i "test|mock" README.md
Length of output: 1496
| static inline std::string URL = "api.github.com"; | ||
| static inline int port = 443; |
There was a problem hiding this comment.
Avoid using global static variables
The use of global static variables URL and port can lead to issues in multithreaded environments or when the values need to be changed dynamically.
Consider encapsulating them within the Github class or passing them as parameters to the methods:
-namespace github {
-
-static inline std::string URL = "api.github.com";
-static inline int port = 443;
-
-} // namespace githubAnd within the Github class:
template <bool https = true>
class Github {
private:
static constexpr const char* URL = "api.github.com";
static constexpr int port = 443;
// ...
};There was a problem hiding this comment.
these values cannot be changed at runtime
There was a problem hiding this comment.
@PhilipDeegan, understood.
✏️ Learnings added
Learnt from: PhilipDeegan
PR: PhilipDeegan/maiken#108
File: inc/maiken/github.hpp:48-49
Timestamp: 2024-10-28T10:48:07.309Z
Learning: In the `maiken` project, when constants cannot be changed at runtime, it's acceptable to define them as `static inline` variables in a namespace.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| if (yaml.root().Type() == 3) { | ||
| if (yaml.root()["ref"]) { | ||
| branch = yaml.root()["ref"].Scalar(); | ||
| b = 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Check for empty YAML root node type
In GET_LATEST_TAG, the code checks if yaml.root().Type() == 3, but it's not clear what Type() == 3 represents.
Use the appropriate YAML::NodeType enumeration for clarity:
if (yaml.root().Type() == YAML::NodeType::Sequence) {
if (!yaml.root().empty() && yaml.root()[0]["ref"]) {
branch = yaml.root()[0]["ref"].Scalar();
b = 1;
}
}| std::vector<std::function<decltype(GET_DEFAULT_BRANCH)>> gets{ | ||
| &GET_DEFAULT_BRANCH, &GET_LATEST_RELEASE, &GET_LATEST_TAG}; |
There was a problem hiding this comment.
Clarify the use of function pointers in the vector
Storing member function pointers in a vector may lead to issues due to the different calling conventions.
Adjust the vector to store std::function objects bound to the class instance or use static methods properly.
std::vector<std::function<bool(std::string const&, std::string const&, std::string&)>> gets{
&Github::GET_DEFAULT_BRANCH, &Github::GET_LATEST_RELEASE, &Github::GET_LATEST_TAG};Ensure that the methods are correctly referenced.
| if (IS_SOLID(repo)) | ||
| repos.push_back(repo); | ||
| else | ||
| for (std::string const& s : Settings::INSTANCE().remoteRepos()) repos.push_back(s + repo); |
There was a problem hiding this comment.
Refine repository URL parsing logic
The current logic for parsing repository URLs assumes specific patterns and may fail for edge cases.
Consider using a regular expression or a dedicated URL parsing library to handle different URL formats more robustly.
For example, you can use std::regex to extract the owner and repo names.
PhilipDeegan
force-pushed
the
next
branch
3 times, most recently
from
ab2517e to
822ab01
Compare
|
closes #40 |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
.github/workflows/build_nix_python.yaml (2)
1-1: Consider using a more descriptive workflow name.The current name "ubuntu-python-latest" focuses on the environment rather than the purpose. Consider a more descriptive name like "Python Build and Test" or "Python CI".
-name: ubuntu-python-latest +name: Python Build and Test
18-18: Optimize max-parallel setting.Since the matrix only includes 2 Python versions (3.11 and 3.12), setting
max-parallel: 4is unnecessarily high. Consider reducing it to 2 for better resource utilization.- max-parallel: 4 + max-parallel: 2inc/maiken/github.hpp (1)
37-43: Consider grouping related includesThe includes could be better organized by grouping standard library headers separately from project-specific headers:
#include "maiken.hpp" #ifdef _MKN_WITH_MKN_RAM_ #include "mkn/kul/yaml.hpp" #include "mkn/ram/http.hpp" #include "mkn/ram/https.hpp" +// Standard library includes #include <chrono> #include <string> #include <thread>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
test/github/__pycache__/mock_api.cpython-312.pycis excluded by!**/*.pyc
📒 Files selected for processing (8)
- .github/workflows/build_nix_python.yaml (1 hunks)
- inc/maiken/github.hpp (2 hunks)
- mkn.yaml (2 hunks)
- src/maiken/scm/github.cpp (0 hunks)
- test/github/mock_api.py (1 hunks)
- test/github/requirements.txt (1 hunks)
- test/github/test.cpp (1 hunks)
- test/github/test_github_api.sh (1 hunks)
💤 Files with no reviewable changes (1)
- src/maiken/scm/github.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- mkn.yaml
- test/github/mock_api.py
- test/github/requirements.txt
- test/github/test.cpp
- test/github/test_github_api.sh
🔇 Additional comments (1)
inc/maiken/github.hpp (1)
Line range hint
67-75: LGTM! Well-designed method signaturesThe method declarations follow good practices with const references and clear parameter naming.
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} | ||
| cancel-in-progress: true | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security configurations to the workflow.
Consider adding the following security-related configurations:
- Explicit permissions to follow the principle of least privilege
- Timeout limits to prevent hanging jobs
Add these configurations after the concurrency block:
cancel-in-progress: true
+ permissions:
+ contents: read
+
+ timeout-minutes: 30Committable suggestion was skipped due to low confidence.
| - run: | | ||
| ./res/ci/nixish_setup.sh && make nix | ||
| sudo cp mkn /usr/bin | ||
| python3 -m pip install -r test/github/requirements.txt | ||
| mkn clean build -p github -dtOa "-fPIC -std=c++17" | ||
| ./test/github/test_github_api.sh |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance CI workflow with best practices.
Consider the following improvements to make the workflow more robust and efficient:
- Add step names for better visibility
- Implement dependency caching
- Split commands into separate steps
- Preserve build artifacts
Here's the suggested implementation:
- - run: |
- ./res/ci/nixish_setup.sh && make nix
- sudo cp mkn /usr/bin
- python3 -m pip install -r test/github/requirements.txt
- mkn clean build -p github -dtOa "-fPIC -std=c++17"
- ./test/github/test_github_api.sh
+ - name: Cache pip dependencies
+ uses: actions/cache@v3
+ with:
+ path: ~/.cache/pip
+ key: ${{ runner.os }}-pip-${{ hashFiles('test/github/requirements.txt') }}
+ restore-keys: |
+ ${{ runner.os }}-pip-
+
+ - name: Setup build environment
+ run: |
+ ./res/ci/nixish_setup.sh && make nix
+ sudo cp mkn /usr/bin
+
+ - name: Install Python dependencies
+ run: python3 -m pip install -r test/github/requirements.txt
+
+ - name: Build project
+ run: mkn clean build -p github -dtOa "-fPIC -std=c++17"
+
+ - name: Run tests
+ run: ./test/github/test_github_api.sh
+
+ - name: Upload build artifacts
+ uses: actions/upload-artifact@v3
+ with:
+ name: build-artifacts
+ path: |
+ bin/
+ lib/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - run: | | |
| ./res/ci/nixish_setup.sh && make nix | |
| sudo cp mkn /usr/bin | |
| python3 -m pip install -r test/github/requirements.txt | |
| mkn clean build -p github -dtOa "-fPIC -std=c++17" | |
| ./test/github/test_github_api.sh | |
| - name: Cache pip dependencies | |
| uses: actions/cache@v3 | |
| with: | |
| path: ~/.cache/pip | |
| key: ${{ runner.os }}-pip-${{ hashFiles('test/github/requirements.txt') }} | |
| restore-keys: | | |
| ${{ runner.os }}-pip- | |
| - name: Setup build environment | |
| run: | | |
| ./res/ci/nixish_setup.sh && make nix | |
| sudo cp mkn /usr/bin | |
| - name: Install Python dependencies | |
| run: python3 -m pip install -r test/github/requirements.txt | |
| - name: Build project | |
| run: mkn clean build -p github -dtOa "-fPIC -std=c++17" | |
| - name: Run tests | |
| run: ./test/github/test_github_api.sh | |
| - name: Upload build artifacts | |
| uses: actions/upload-artifact@v3 | |
| with: | |
| name: build-artifacts | |
| path: | | |
| bin/ | |
| lib/ |
| template <bool https> | ||
| bool Github<https>::GET_LATEST_RELEASE(std::string const& owner, std::string const& repo, | ||
| std::string& branch) { | ||
| bool b = 0; | ||
| int retry = 3; | ||
| std::stringstream ss; | ||
| ss << "repos/" << owner << "/" << repo << "/releases/latest"; | ||
|
|
||
| while (retry-- > 0) { | ||
| request(ss.str()) | ||
| .withHeaders({{"User-Agent", "Mozilla not a virus"}, {"Accept", "application/json"}}) | ||
| .withResponse([&b, &branch](auto const& r) { | ||
| if (r.status() == 200) // | ||
| try { | ||
| mkn::kul::yaml::String const yaml(r.body()); | ||
| if (yaml.root()["tag_name"]) { | ||
| branch = yaml.root()["tag_name"].Scalar(); | ||
| b = 1; | ||
| } | ||
| } catch (YAML::Exception const&) { | ||
| KLOG(ERR) << "maiken::Github::GET_LATEST_RELEASE invalid response received."; | ||
| } | ||
| }) | ||
| .send(); | ||
| if (b) return b; | ||
| KLOG(ERR) << "maiken::Github::GET_LATEST_RELEASE failed - retrying"; | ||
| using namespace std::chrono_literals; | ||
| std::this_thread::sleep_for(50ms); | ||
| } | ||
| return b; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting common retry logic
The retry mechanism is duplicated across multiple methods. Consider extracting it into a reusable helper.
template <typename F>
static bool withRetry(const char* operation, F&& func) {
int retry = MAX_RETRY_ATTEMPTS;
while (retry-- > 0) {
if (func()) return true;
KLOG(ERR) << "maiken::Github::" << operation << " failed - attempt "
<< (MAX_RETRY_ATTEMPTS - retry) << "/" << MAX_RETRY_ATTEMPTS;
std::this_thread::sleep_for(RETRY_DELAY);
}
return false;
}Usage example:
bool Github<https>::GET_LATEST_RELEASE(...) {
std::stringstream ss;
ss << "repos/" << owner << "/" << repo << "/releases/latest";
bool success = false;
return withRetry("GET_LATEST_RELEASE", [&]() {
request(ss.str())
.withHeaders({...})
.withResponse([&success, &branch](auto const& r) {
// existing response handling
success = true;
})
.send();
return success;
});
}| if (IS_SOLID(repo)) | ||
| repos.push_back(repo); | ||
| else | ||
| for (std::string const& s : Settings::INSTANCE().remoteRepos()) repos.push_back(s + repo); | ||
| for (std::string const& s : repos) { | ||
| if (s.find("github.com") != std::string::npos) { | ||
| std::string owner = s.substr(s.find("github.com") + 10); | ||
| if (owner[0] != '/' && owner[0] != ':') { | ||
| KERR << "Repo \"" << s << "\" is invalid - skipping"; | ||
| continue; | ||
| } | ||
| owner.erase(0, 1); | ||
| if (owner.find("/") != std::string::npos) owner = owner.substr(0, owner.find("/")); | ||
|
|
||
| if (owner.empty()) { | ||
| KERR << "Invalid attempt to perform github lookup"; | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
Improve GitHub URL parsing and error handling
The current URL parsing logic could be more robust and provide better error messages.
Consider using a more structured approach:
struct GitHubRepo {
std::string owner;
std::string name;
static std::optional<GitHubRepo> parse(const std::string& url) {
static const std::regex github_url_regex(
R"(github\.com[/:]([\w.-]+)/([\w.-]+)(?:\.git)?(?:/)?$)");
std::smatch matches;
if (std::regex_search(url, matches, github_url_regex)) {
return GitHubRepo{matches[1], matches[2]};
}
return std::nullopt;
}
};Usage:
- std::string owner = s.substr(s.find("github.com") + 10);
- if (owner[0] != '/' && owner[0] != ':') {
- KERR << "Repo \"" << s << "\" is invalid - skipping";
- continue;
- }
- owner.erase(0, 1);
+ auto parsed_repo = GitHubRepo::parse(s);
+ if (!parsed_repo) {
+ KERR << "Invalid GitHub URL format: " << s;
+ continue;
+ }| if (b == 1) { | ||
| auto bits(mkn::kul::String::SPLIT(branch, "/")); | ||
| branch = bits[bits.size() - 1]; | ||
| return b; | ||
| } |
There was a problem hiding this comment.
Improve branch name extraction robustness
The current branch name extraction assumes the last segment is the tag name, but this might not always be true.
Consider using a more robust approach:
- if (b == 1) {
- auto bits(mkn::kul::String::SPLIT(branch, "/"));
- branch = bits[bits.size() - 1];
- return b;
- }
+ if (b == 1) {
+ if (branch.empty()) return false;
+ size_t last_slash = branch.find_last_of('/');
+ if (last_slash != std::string::npos) {
+ branch = branch.substr(last_slash + 1);
+ }
+ return !branch.empty();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (b == 1) { | |
| auto bits(mkn::kul::String::SPLIT(branch, "/")); | |
| branch = bits[bits.size() - 1]; | |
| return b; | |
| } | |
| if (b == 1) { | |
| if (branch.empty()) return false; | |
| size_t last_slash = branch.find_last_of('/'); | |
| if (last_slash != std::string::npos) { | |
| branch = branch.substr(last_slash + 1); | |
| } | |
| return !branch.empty(); | |
| } |
| template <bool https> | ||
| bool Github<https>::GET_DEFAULT_BRANCH(std::string const& owner, std::string const& repo, | ||
| std::string& branch) { | ||
| bool b = 0; | ||
| int retry = 3; | ||
| std::stringstream ss; | ||
| ss << "repos/" << owner << "/" << repo; | ||
| while (retry-- > 0) { | ||
| request(ss.str()) | ||
| .withHeaders({{"User-Agent", "Mozilla not a virus"}, {"Accept", "application/json"}}) | ||
| .withResponse([&b, &branch](auto const& r) { | ||
| if (r.status() == 200) // | ||
| try { | ||
| mkn::kul::yaml::String const yaml(r.body()); | ||
| KLOG(OTH) << "Github API default branch response: " << r.body(); | ||
| if (yaml.root() && yaml.root()["default_branch"]) { | ||
| branch = yaml.root()["default_branch"].Scalar(); | ||
| b = 1; | ||
| } | ||
| } catch (YAML::Exception const&) { | ||
| KLOG(ERR) << "maiken::Github::GET_DEFAULT_BRANCH invalid response received."; | ||
| } | ||
| }) | ||
| .send(); | ||
| if (b) return b; | ||
| KLOG(ERR) << "maiken::Github::GET_DEFAULT_BRANCH failed - retrying"; | ||
| using namespace std::chrono_literals; | ||
| std::this_thread::sleep_for(50ms); | ||
| } | ||
| return b; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving retry mechanism and error handling
- The retry count should be a named constant
- Error handling could be more informative
+static constexpr int MAX_RETRY_ATTEMPTS = 3;
+static constexpr auto RETRY_DELAY = std::chrono::milliseconds(50);
bool Github<https>::GET_DEFAULT_BRANCH(...) {
bool b = 0;
- int retry = 3;
+ int retry = MAX_RETRY_ATTEMPTS;
std::stringstream ss;
ss << "repos/" << owner << "/" << repo;
while (retry-- > 0) {
// ... existing code ...
if (b) return b;
- KLOG(ERR) << "maiken::Github::GET_DEFAULT_BRANCH failed - retrying";
+ KLOG(ERR) << "maiken::Github::GET_DEFAULT_BRANCH failed - attempt "
+ << (MAX_RETRY_ATTEMPTS - retry) << "/" << MAX_RETRY_ATTEMPTS;
using namespace std::chrono_literals;
- std::this_thread::sleep_for(50ms);
+ std::this_thread::sleep_for(RETRY_DELAY);
}
return b;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| template <bool https> | |
| bool Github<https>::GET_DEFAULT_BRANCH(std::string const& owner, std::string const& repo, | |
| std::string& branch) { | |
| bool b = 0; | |
| int retry = 3; | |
| std::stringstream ss; | |
| ss << "repos/" << owner << "/" << repo; | |
| while (retry-- > 0) { | |
| request(ss.str()) | |
| .withHeaders({{"User-Agent", "Mozilla not a virus"}, {"Accept", "application/json"}}) | |
| .withResponse([&b, &branch](auto const& r) { | |
| if (r.status() == 200) // | |
| try { | |
| mkn::kul::yaml::String const yaml(r.body()); | |
| KLOG(OTH) << "Github API default branch response: " << r.body(); | |
| if (yaml.root() && yaml.root()["default_branch"]) { | |
| branch = yaml.root()["default_branch"].Scalar(); | |
| b = 1; | |
| } | |
| } catch (YAML::Exception const&) { | |
| KLOG(ERR) << "maiken::Github::GET_DEFAULT_BRANCH invalid response received."; | |
| } | |
| }) | |
| .send(); | |
| if (b) return b; | |
| KLOG(ERR) << "maiken::Github::GET_DEFAULT_BRANCH failed - retrying"; | |
| using namespace std::chrono_literals; | |
| std::this_thread::sleep_for(50ms); | |
| } | |
| return b; | |
| } | |
| static constexpr int MAX_RETRY_ATTEMPTS = 3; | |
| static constexpr auto RETRY_DELAY = std::chrono::milliseconds(50); | |
| template <bool https> | |
| bool Github<https>::GET_DEFAULT_BRANCH(std::string const& owner, std::string const& repo, | |
| std::string& branch) { | |
| bool b = 0; | |
| int retry = MAX_RETRY_ATTEMPTS; | |
| std::stringstream ss; | |
| ss << "repos/" << owner << "/" << repo; | |
| while (retry-- > 0) { | |
| request(ss.str()) | |
| .withHeaders({{"User-Agent", "Mozilla not a virus"}, {"Accept", "application/json"}}) | |
| .withResponse([&b, &branch](auto const& r) { | |
| if (r.status() == 200) // | |
| try { | |
| mkn::kul::yaml::String const yaml(r.body()); | |
| KLOG(OTH) << "Github API default branch response: " << r.body(); | |
| if (yaml.root() && yaml.root()["default_branch"]) { | |
| branch = yaml.root()["default_branch"].Scalar(); | |
| b = 1; | |
| } | |
| } catch (YAML::Exception const&) { | |
| KLOG(ERR) << "maiken::Github::GET_DEFAULT_BRANCH invalid response received."; | |
| } | |
| }) | |
| .send(); | |
| if (b) return b; | |
| KLOG(ERR) << "maiken::Github::GET_DEFAULT_BRANCH failed - attempt " | |
| << (MAX_RETRY_ATTEMPTS - retry) << "/" << MAX_RETRY_ATTEMPTS; | |
| using namespace std::chrono_literals; | |
| std::this_thread::sleep_for(RETRY_DELAY); | |
| } | |
| return b; | |
| } |
Summary by CodeRabbit
Release Notes
New Features
githubnamespace with aGithubclass for enhanced interaction with the GitHub API.Bug Fixes
Documentation
githubprofile and dependencies.Tests
Chores